fix: address review comments from PRs #2604 and #2619#2634
Conversation
PR #2604 (egress hardening) follow-ups: - run_arxiv_download_adapter: offload blocking Path.write_bytes to a thread (event-loop stall under load) - tokenizer_resolver._http_post: allow_redirects=False — these POSTs carry provider credentials; following a cross-origin redirect would resend secrets to the redirect target (test contract updated) - tokenizer_resolver: log (not suppress) failures restoring encoding.timeout_seconds in the runtime-probe finally block - egress remediation plan: TASK-12138 -> TASK-12146 traceability fix PR #2619 (ACP reconnect cleanup) follow-ups: - extract reconnect-replay lifecycle into core: ws_broadcaster.start_reconnect_replay / stop_reconnect_replay (endpoint no longer owns broadcaster workflow logic) - cleanup failures are now logged instead of contextlib.suppress'ed - WSBroadcaster.__init__: document the consumer_id parameter - new tests: drop redundant @pytest.mark.asyncio (asyncio_mode=auto), add module-level unit marker to test_ws_broadcaster.py, complete type hints on new test helpers - test_determine_permission_tier_batch: was red on dev — pinned pre-hardening behavior (write->batch); updated for the hardened contract (write/modify->individual, fallback->batch) Verified: 963/963 tests in tests/Agent_Client_Protocol/, 117/117 in tokenizer/research-adapter files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoFollow up audit-fix reviews: safer redirects, async file I/O, ACP replay lifecycle helpers
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements and security hardening measures, including refactoring the WebSocket reconnect-replay logic to use helper functions with explicit error logging, disabling redirects on tokenizer POST requests to prevent credential leakage, and offloading blocking file writes to a separate thread using asyncio.to_thread. Additionally, test suites have been updated to align with permission tier hardening. Feedback on these changes identifies a potential resource leak in start_reconnect_replay if add_connection fails after the broadcaster has started, and suggests wrapping the call in a try...except block to ensure the broadcaster is stopped on failure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async def start_reconnect_replay( | ||
| bus: SessionEventBus, | ||
| *, | ||
| conn_id: str, | ||
| send_callback: SendCallback, | ||
| from_sequence: int, | ||
| ) -> WSBroadcaster: | ||
| """Create and start a dedicated broadcaster that replays missed events. | ||
|
|
||
| Used on WebSocket reconnect: a per-connection broadcaster (unique | ||
| ``consumer_id`` so concurrent reconnects don't clobber each other's bus | ||
| subscriptions) is subscribed to *bus* and replays buffered events from | ||
| *from_sequence* to *send_callback*. Callers must pair this with | ||
| :func:`stop_reconnect_replay` on disconnect. | ||
| """ | ||
| broadcaster = WSBroadcaster(consumer_id=f"ws_broadcaster:{conn_id}") | ||
| await broadcaster.start(bus) | ||
| await broadcaster.add_connection( | ||
| conn_id=conn_id, | ||
| send_callback=send_callback, | ||
| from_sequence=from_sequence, | ||
| ) | ||
| return broadcaster |
There was a problem hiding this comment.
If add_connection raises an exception after broadcaster.start(bus) has succeeded, the broadcaster will remain active and subscribed to the event bus, leading to a resource/memory leak. Since the caller (agent_client_protocol.py) only receives the broadcaster reference upon successful return, it won't be able to clean it up in its finally block. Wrapping add_connection in a try...except block to stop the broadcaster on failure ensures proper cleanup.
async def start_reconnect_replay(
bus: SessionEventBus,
*,
conn_id: str,
send_callback: SendCallback,
from_sequence: int,
) -> WSBroadcaster:
"""Create and start a dedicated broadcaster that replays missed events.
Used on WebSocket reconnect: a per-connection broadcaster (unique
``consumer_id`` so concurrent reconnects don't clobber each other's bus
subscriptions) is subscribed to *bus* and replays buffered events from
*from_sequence* to *send_callback*. Callers must pair this with
:func:`stop_reconnect_replay` on disconnect.
"""
broadcaster = WSBroadcaster(consumer_id=f"ws_broadcaster:{conn_id}")
await broadcaster.start(bus)
try:
await broadcaster.add_connection(
conn_id=conn_id,
send_callback=send_callback,
from_sequence=from_sequence,
)
except Exception:
await broadcaster.stop()
raise
return broadcasterThere was a problem hiding this comment.
Addressed in dd8660b: add_connection is now wrapped so a mid-replay failure stops the broadcaster (best-effort, logged) before re-raising; regression test test_start_reconnect_replay_cleans_up_when_registration_fails asserts no leaked bus subscriber.
Code Review by Qodo
Context used✅ Compliance rules (platform):
74 rules 1.
|
… cleanup logs Addresses PR #2634 review comments: - start_reconnect_replay: if add_connection raises mid-replay, stop the broadcaster (best-effort, logged) before re-raising — otherwise the bus subscription and consume task leak because the caller never receives the reference to clean up. Regression test added. - stop_reconnect_replay + failure path: logger.bind(action, conn_id, consumer_id) for correlatable teardown warnings. - start_reconnect_replay docstring: full Args/Returns/Raises sections. Verified: 964/964 tests in tests/Agent_Client_Protocol/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Task 4 nightly shuffle: use -p pytest_randomly (full module name), not the entry-point alias, per review — more reliable under PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 (gemini review) - Add tracked follow-up: central http_client cross-origin redirect header stripping (qodo Option B from PR #2604), superseding the per-caller allow_redirects=False mitigation from PR #2634 - fake-indexeddb package.json comment was already addressed in a prior revision Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Follow-up addressing every unresolved reviewer comment from the two audit-fix PRs merged earlier today (the client-reuse and trivial-test comments were already fixed in-branch before merge; the
nonlocalsuggestion on #2619 was correctly rejected by the author).From PR #2604 (egress hardening)
search.py:207):run_arxiv_download_adapternow offloadsPath.write_bytesviaasyncio.to_thread. (Theimport arxivbranch's blocking library calls are pre-existing and out of scope.)tokenizer_resolver.py:664, security):_http_postnow sendsallow_redirects=False— these POSTs carryAuthorization/x-api-keyand the central redirect loop doesn't strip headers cross-origin. Regression test contract updated. The central fix (strip/partition sensitive headers on cross-origin redirects inhttp_clientfetch/afetch) is qodo's preferred Option B and remains open — larger change, deserves its own PR.tokenizer_resolver.py:895): restoringencoding.timeout_secondsnow logs a warning on failure instead ofsuppress(Exception).TASK-12138→TASK-12146.From PR #2619 (ACP reconnect cleanup)
ws_broadcaster.start_reconnect_replay()/stop_reconnect_replay(); the endpoint just calls them.agent_client_protocol.py:1343): teardown failures are now logged instop_reconnect_replay, never silently dropped.WSBroadcaster.__init__documentsconsumer_id.asynciomarker + missing type hints on new tests (qodo): dropped redundant@pytest.mark.asynciofrom the new tests (asyncio_mode=auto), added module-levelpytestmark = pytest.mark.unittotest_ws_broadcaster.py, completed type hints on the new test helpers.Bonus
test_determine_permission_tier_batchwas red on dev: it pinned pre-hardening behavior (write→ batch). Updated for the hardened contract (write/modify→ individual; true fallback tools → batch).Verification
tests/Agent_Client_Protocol/: 963/963 pass locallytest_tokenizer_resolver_unit.py+test_research_adapters.py: 117/117 passPYTEST_DISABLE_PLUGIN_AUTOLOAD=1+-p pytest_asyncio.pluginper repo convention🤖 Generated with Claude Code
Summary by cubic
Follow-up to #2604 (egress hardening) and #2619 (ACP reconnect) that closes remaining review comments. Prevents credential leaks on redirects, removes event-loop blocking I/O, and hardens reconnect replay with lifecycle helpers, error cleanup, and clearer logs/tests. Updates plan doc to reference TASK-12146 for audit verification.
Bug Fixes
allow_redirects=Falseto avoid sending auth headers on redirects; tests updated.Path.write_bytesviaasyncio.to_threadto avoid event-loop stalls.start_reconnect_replaynow stops the broadcaster ifadd_connectionfails mid-replay to prevent leaked bus subscriptions; regression test added.TASK-12146.Refactors
start_reconnect_replay/stop_reconnect_replay; endpoint now only calls them.action/conn_id/consumer_id(no silent suppress).WSBroadcaster.__init__documentsconsumer_id; tests drop redundant@pytest.mark.asyncio, add unit marker, and complete type hints.Written for commit dd8660b. Summary will update on new commits.